Serpent material temps #178

yardasol · 2022-11-28T18:41:05Z

Summary of changes

This PR adds machinery to maintain cross section data consistency between depletion steps when using SaltProc with Serpent2. Previously, the fix card was hardcoded to a library id of 09c, but now SaltProc checks for the fix option which tells Serpent which library IDs to use for a material, and uses the same value for the fix option in the entire simulation.

Specifically, this PR:

Adds two functions to SerpentDepcode:
- get_burnable_materials_file(), a helper function for create_runtime_matfile()
- get_burnable_material_card_data() that scrapes the template material file and stores the material cards in an attribute

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Required for Merging

Associated Issues and PRs

PR: Supercedes Fix hardcoded material temperatures #154
Issue: closes [Bug]: Fuel Material Temperature Hardcoded (maybe?) in saltproc_mat #146

Associated Developers

Dev: @

Checklist for Reviewers

Reviewers should use this link to get to the
Review Checklist before they begin their review.

…al file

abachma2

Hi @yardasol. This looks good overall. I have a few questions to be answered. For your integration tests, it looks like you've only added one test, but your function changes include if loops. Do you think you need tests to cover what happens in the if loops?

abachma2 · 2022-12-07T17:48:25Z

saltproc/serpent_depcode.py

+        shutil.copy2(absolute_path, self.runtime_matfile)
+        return [line.replace(burnable_materials_path, self.runtime_matfile) for line in file_lines]
+
+    def get_burnable_materials_file(self, file_lines):


Do you need a docstring for this function?

If this is a private function, I will accept not having a docstring. Since users should never interact with it. In this case, the code is documentation. However, if it is a private function it should have a single underscore at the beginning of the method name.

abachma2 · 2022-12-07T17:48:37Z

saltproc/serpent_depcode.py

-        # Create file with path for SaltProc rewritable iterative material file
-        shutil.copy2(absolute_path, self.runtime_matfile)
-        return [line.replace(burnable_materials_path, self.runtime_matfile) for line in file_lines]
+    def get_burnable_material_card_data(self, file_lines):


Do you need a docstring for this function?

Same comment as above.

abachma2 · 2022-12-07T17:50:16Z

saltproc/serpent_depcode.py

-                         mat.vol))
+                mat_card, card_volume_idx = self.burnable_material_card_data[name]
+                mat_card[2] = str(-mat.density)
+                mat_card[card_volume_idx] = "%7.5E" % mat.vol


This feels like a hardcoded value. Is this the case?

No this is string formatting

That's what I was thinking. Just wanted to make sure it wasn't hard coding any values.

samgdotson

This seems fine, just answer @abachma2's questions.

samgdotson · 2022-12-07T18:10:58Z

saltproc/serpent_depcode.py

+        shutil.copy2(absolute_path, self.runtime_matfile)
+        return [line.replace(burnable_materials_path, self.runtime_matfile) for line in file_lines]
+
+    def get_burnable_materials_file(self, file_lines):


If this is a private function, I will accept not having a docstring. Since users should never interact with it. In this case, the code is documentation. However, if it is a private function it should have a single underscore at the beginning of the method name.

samgdotson · 2022-12-07T18:11:10Z

saltproc/serpent_depcode.py

-        # Create file with path for SaltProc rewritable iterative material file
-        shutil.copy2(absolute_path, self.runtime_matfile)
-        return [line.replace(burnable_materials_path, self.runtime_matfile) for line in file_lines]
+    def get_burnable_material_card_data(self, file_lines):


Same comment as above.

yardasol · 2022-12-07T18:45:41Z

@abachma2

For your integration tests, it looks like you've only added one test, but your function changes include if loops. Do you think you need tests to cover what happens in the if loops?

You are correct, I absolutely should.

abachma2

Thanks for making these latest changes @yardasol. I have one other comment about the tests. Once you write more tests to cover the if loops in your functions I will approve and merge this PR.

abachma2 · 2022-12-07T19:06:41Z

tests/integration_tests/file_interface_serpent/test.py

@@ -36,6 +36,54 @@ def test_runtime_input_from_template(serpent_depcode, msr):
    file_data = serpent_depcode.create_runtime_matfile(file_data)
    assert file_data[0].split()[-1] == '\"' + \
        serpent_depcode.runtime_matfile + '\"'
+
+    # get_burnable_material_card_data
+    _burnable_material_card_data = {'fuel':


Maybe this is just a knowledge gap on my part, but why did you add the underscore here? This looks like a variable to me, not a call of that private function?

It's a private attribute

I get that for in line 79. But even when you define it here and use it in line 80? It's still a private attribute in those locations?

no, this is just for naming consistency. Should I name it something else?

I would recommend changing the same some when you're not calling the attribute. It was a little confusing to me to have the two things you're comparing be named the same thing. I like to use the exp and obs (as in expected and observed) that Katy uses in her book for my tests. But your choice as to what you change it to.

Okay I'll do this

abachma2

Thanks for adding the unit tests @yardasol. I think they make the test suite more robust. It looks like your unit tests are just testing that the proper error is raised. Do you also have a test in there for when the inputs have the correct information and the function should execute as expected?

abachma2 · 2022-12-07T19:29:11Z

tests/integration_tests/file_interface_serpent/test.py

@@ -36,6 +36,54 @@ def test_runtime_input_from_template(serpent_depcode, msr):
    file_data = serpent_depcode.create_runtime_matfile(file_data)
    assert file_data[0].split()[-1] == '\"' + \
        serpent_depcode.runtime_matfile + '\"'
+
+    # get_burnable_material_card_data
+    _burnable_material_card_data = {'fuel':


I get that for in line 79. But even when you define it here and use it in line 80? It's still a private attribute in those locations?

yardasol · 2022-12-07T19:58:35Z

Do you also have a test in there for when the inputs have the correct information and the function should execute as expected?

Yes, that machinery is added to the integration test test_file_interface_serpent

abachma2 · 2022-12-07T20:02:47Z

Do you also have a test in there for when the inputs have the correct information and the function should execute as expected?

Yes, that machinery is added to the integration test test_file_interface_serpent

And you think that having that only in an integration test is sufficient? It doesn't need its own unit test? I'm not expecting a certain answer, I'm genuinely asking.

yardasol · 2022-12-07T20:08:59Z

And you think that having that only in an integration test is sufficient? It doesn't need its own unit test?

It should be sufficient. As long as the user provides correct input paths none of the errors should be raised, and the code will run correctly. Maybe @munkm can chime in?

yardasol · 2022-12-09T18:30:45Z

@samgdotson bump

munkm · 2022-12-15T19:29:18Z

ok @yardasol and @abachma2 I took a look. I think generally the integration test is probably good enough for this case. However, having a unit test that checks the specific function like Amanda brought up would be even better. Users are good at generating all sorts of weird corner cases and this integration test isn't going to capture that (or any improvements to the tooling that detects the input paths). Are errors raised consistently for all sorts of weird iterations of paths (I don't see that tested here)? Are the expected errors raised? If a user comes up with a case where they enter a path wrong and the error isn't raised, where would they modify this integration test to include it? I don't see an obvious spot in test_file_interface_serpent. I don't see an assertion that an error is raised with a wrong configuration either. Am I missing it?

yardasol · 2022-12-15T22:43:50Z

@munkm

Are errors raised consistently for all sorts of weird iterations of paths (I don't see that tested here)?

If a path is bad, Python should raise a FileNotFoundError. My personal opinion is that it is out-of-scope to try to test fundamental Python machinery in a program built on top of it.

Are the expected errors raised?

See this test.

If a user comes up with a case where they enter a path wrong and the error isn't raised, where would they modify this integration test to include it?

Again, this is more of an issue with python itself than it is with SaltProc.

I don't see an assertion that an error is raised with a wrong configuration either. Am I missing it?

I'm not sure what you mean by this. Are you talking about the return message? Or are you talking about assertions than a particular error is raised? If the latter, see the above link.

yardasol · 2023-01-02T18:18:20Z

@munkm bump

samgdotson

This seems fine, apologies for the delay.

…material-temps Serpent material temps beb9b1d

…ent-material-temps Serpent material temps beb9b1d

munkm · 2023-01-11T20:17:20Z

I'm good with this merge now that there's an open issue! Thanks for doing that @samgdotson

munkm · 2023-01-11T20:26:08Z

But please next time check with the other people who have started review before merging. It wasn't clear if @abachma2 had approved yet.

yardasol force-pushed the serpent-material-temps branch from 35101a0 to 32c02be Compare November 28, 2022 18:44

yardasol added 4 commits November 28, 2022 12:49

add machinery to use the original material card in the runtime materi…

32c02be

…al file

add missing comma

0f690a3

syntax fixes

42a1253

fix nonpassing tests

5586a09

yardasol mentioned this pull request Nov 29, 2022

Fix hardcoded material temperatures #154

Closed

17 tasks

yardasol force-pushed the serpent-material-temps branch 2 times, most recently from ebf2dc4 to 5f84d01 Compare December 6, 2022 17:24

yardasol and others added 5 commits December 6, 2022 11:30

Merge branch 'master' into serpent-material-temps

5f84d01

update release notes

d0c5b55

get_material_data() -> get_burnable_material_card_data()

df1c026

added test for burnable_material_card_data attribute

d51b204

fix writing of runtime material file

f0e9b22

yardasol requested review from abachma2 and samgdotson December 7, 2022 17:40

yardasol marked this pull request as ready for review December 7, 2022 17:40

add check for fix option in mat card

d206857

abachma2 reviewed Dec 7, 2022

View reviewed changes

samgdotson requested changes Dec 7, 2022

View reviewed changes

make new functions private

08d94a5

abachma2 reviewed Dec 7, 2022

View reviewed changes

yardasol added 2 commits December 7, 2022 13:37

clean up new functions

aa46951

add tests for if statements in new functions

0c17f30

yardasol requested review from abachma2 and samgdotson December 7, 2022 19:37

abachma2 reviewed Dec 7, 2022

View reviewed changes

yardasol mentioned this pull request Dec 8, 2022

[Meta] Work order for remaining PRs/Issues for OpenMC support #163

Closed

remove '_' prefix in test variable

35cdb8d

yardasol mentioned this pull request Jan 6, 2023

Feature: Add machinery for verifying file and directory paths #183

Open

samgdotson approved these changes Jan 9, 2023

View reviewed changes

samgdotson merged commit beb9b1d into arfc:master Jan 9, 2023

github-actions bot pushed a commit that referenced this pull request Jan 9, 2023

update docs at commit: Merge pull request #178 from yardasol/serpent-…

d726841

…material-temps Serpent material temps beb9b1d

github-actions bot pushed a commit to khurrumsaleem/saltproc that referenced this pull request Jan 10, 2023

update docs at commit: Merge pull request arfc#178 from yardasol/serp…

9fe4c83

…ent-material-temps Serpent material temps beb9b1d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serpent material temps #178

Serpent material temps #178

yardasol commented Nov 28, 2022 •

edited

Loading

abachma2 left a comment

abachma2 Dec 7, 2022

samgdotson Dec 7, 2022

abachma2 Dec 7, 2022

samgdotson Dec 7, 2022

abachma2 Dec 7, 2022

samgdotson Dec 7, 2022

abachma2 Dec 7, 2022

samgdotson left a comment

samgdotson Dec 7, 2022

samgdotson Dec 7, 2022

yardasol commented Dec 7, 2022

abachma2 left a comment

abachma2 Dec 7, 2022

yardasol Dec 7, 2022

abachma2 Dec 7, 2022

yardasol Dec 7, 2022

abachma2 Dec 7, 2022

yardasol Dec 7, 2022

abachma2 left a comment

abachma2 Dec 7, 2022

yardasol commented Dec 7, 2022

abachma2 commented Dec 7, 2022

yardasol commented Dec 7, 2022

yardasol commented Dec 9, 2022

munkm commented Dec 15, 2022

yardasol commented Dec 15, 2022

yardasol commented Jan 2, 2023

samgdotson left a comment

munkm commented Jan 11, 2023

munkm commented Jan 11, 2023

Serpent material temps #178

Serpent material temps #178

Conversation

yardasol commented Nov 28, 2022 • edited Loading

Summary of changes

Types of changes

Required for Merging

Associated Issues and PRs

Associated Developers

Checklist for Reviewers

abachma2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samgdotson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yardasol commented Dec 7, 2022

abachma2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abachma2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yardasol commented Dec 7, 2022

abachma2 commented Dec 7, 2022

yardasol commented Dec 7, 2022

yardasol commented Dec 9, 2022

munkm commented Dec 15, 2022

yardasol commented Dec 15, 2022

yardasol commented Jan 2, 2023

samgdotson left a comment

Choose a reason for hiding this comment

munkm commented Jan 11, 2023

munkm commented Jan 11, 2023

yardasol commented Nov 28, 2022 •

edited

Loading